-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Get examples working at 1.3 #4
Conversation
This breaks Cannon.js examples.
This solves the issue with deprecation of THREE.Geometry in A-Frame 1.2.0
Haven't tested this yet...
Live version of the code on this branch can be accessed here: |
src/components/body/body.js
Outdated
@@ -329,7 +336,7 @@ var Body = { | |||
} else { | |||
q1.copy(body.quaternion); | |||
parentEl.object3D.getWorldQuaternion(q2); | |||
q1.premultiply(q2.invert()); | |||
q1.premultiply(q2.inverse()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why you did this change.
Quaternion inverse()
was renamed to invert()
since three r125. I think you want to keep invert()
here and in other places where you did this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching this. I was getting "invert() is not a function" errors. Must have been somehow using an old version of THREE that didn't support invert() yet? I'll investigate further.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also "aframe": ">=0.5.0"
in peerDependencies
, you may have an issue with that because it doesn't match the version in devDependencies
, so it's actually running with a really old super-three that is included in aframe 0.5.0?
I'm not sure why we have three
in devDependencies here, we don't seem to have imports from three in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reverted this. Expected it to break the tests since the tests still run against A-Frame 1.1.0, but it didn't - I guess because there is so little actual test coverage.
package.json
Outdated
@@ -71,7 +71,7 @@ | |||
"replace": "^1.2.0", | |||
"sinon": "^2.4.1", | |||
"sinon-chai": "^2.14.0", | |||
"three": "^0.123.0", | |||
"three": "^0.125.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably want to test against aframe ^1.3.0 and three ^0.137.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, sensible. I was going for the minimum required change here, but if I don't introduce new issues upgrading further I'll do what you suggest. (and if I do, I guess we'll need to fix them anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and if you can, can you please test the examples with an aframe master build as well? aframe master is currently using three r144, you may catch some warnings to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so the test issue that I hit here: #4 (comment) turns out to appear whenever the tests use A-Frame > 1.1.0.
I don't understand this but moving tests to A-Frame 1.2.0+ breaks them as the browserify build step fails.
I'll raise a separate issue for that, but for now it's better to have tests that run cleanly against 1.1.0 than tests that don't work at all against 1.2.0 or 1.3.0.
That said it is a little moot as the tests barely cover anything anyway...!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI aframe 1.1.0 is using super-three 0.123.1 so it's Quaternion.inverse.
aframe 1.2.0 is using super-three 0.125.0 so it's Quaternion.invert. so aframe 1.2.0 should be the minimum we support here. But yeah because the tests are not covering much of the code, it probably doesn't fail with aframe 1.1.0. :)
I'm ok with keeping 1.1.0 here for now in this PR, we should probably move from browserify to webpack anyway in a separate PR like I did with aframe and networked-aframe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate issue raised here:
#5
If moving to webpack will make this obsolete that sounds like a good option too.
We should replace all https://rawgit.com urls in the examples by jsdelivr or unpkg. |
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these look good, just a matter of making those last recommendations from vincent. I did have on question here, but I assume there's a reason.
So, we're waiting to merge on:
Then Vincent will rework to webpack in a separate PR. That right? |
Yes. |
Also...
Examples are all now clean. I think tis is now ready to merge. Just wwaiting to hear if anyone has any concerns, as it's ended up quite a large PR. |
Looks good to me. At this point, it's obviously a massive improvement over what was before it, we've all looked over it, and the best way to find any more issues is to merge it and use it. Thanks for this excellent stuff, there's not many people out there who are able and willing to do this. Very excited to use this in a current project of mine immediately! |
@@ -69,7 +69,7 @@ module.exports = AFRAME.registerComponent("ammo-constraint", { | |||
|
|||
const bodyTransform = body | |||
.getCenterOfMassTransform() | |||
.invert() | |||
.inverse() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's super sneaky. Was indeed confusing. But inverse() does seem to be what's actually available here: https://github.com/kripken/ammo.js/blob/99d0ec0b1e26d7ccc13e013caba8e8a5c98d953b/bullet/src/BulletDynamics/ConstraintSolver/btGeneric6DofSpringConstraint.cpp#L178-L179
You might add a comment here about this, because this could be confused by someone else in the future.
All good, thanks @diarmidmackenzie |
Fixes as follows:
This PR also includes the changes submitted under PR#1 (though they aren't actually needed to get 1.3.0 examples working). Can revert those if we want to merge this without merging that.
#1